Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/MeshImporterExporter_test.cpp (2)

1617-1670: Exercise the exact transform and animation data this PR is supposed to preserve.

RoundTrip_TwoEntities_PreservesTransforms never sets rotation and only checks one scale axis, while RoundTrip_SkeletonEntity_PreservesAnimations only asserts >= 1 animation after reload. Those tests will still pass if orientation is dropped or animation filtering regresses. Add a non-identity rotation and assert all scale components, then verify the expected animation count/name that should round-trip.

Also applies to: 1716-1749

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter_test.cpp` around lines 1617 - 1670, The test
RoundTrip_TwoEntities_PreservesTransforms must exercise and assert the full
transform: set a non-identity rotation on SceneNode1 (use the scene node API
used elsewhere, e.g., setOrientation/setRotation or whichever method exists on
SceneNode), assert all three scale components (x,y,z) on reload rather than only
x, and verify the rotation was preserved (compare the reloaded node's
orientation/rotation to the original within a small tolerance). Likewise, update
RoundTrip_SkeletonEntity_PreservesAnimations to assert the exact expected
animation count and specific animation names (not just >= 1) after reimport to
ensure animation filtering/round-trip is preserved. Use the existing symbols
Manager, SceneNode (sn1/sn2), MeshImporterExporter::sceneExporter/sceneImporter,
and the test names to locate and change the tests.

---

1672-1701: This test does not prove material deduplication yet.

MaterialDedup_SharedMaterial_ExportedOnce currently only reimports the scene and recounts nodes, so duplicated material entries in the written .scene.gltf would still pass. Since this path already exports text glTF, please assert the exported materials array stays at one entry, or at least confirm both reloaded entities resolve to the same material resource.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter_test.cpp` around lines 1672 - 1701, The test
currently only checks node count but not material deduplication; update
SceneSaveLoadTest::MaterialDedup_SharedMaterial_ExportedOnce to assert
deduplication by either (A) reading the exported text glTF at sceneFile after
MeshImporterExporter::sceneExporter and parsing its JSON to assert the
"materials" array has size 1, or (B) after MeshImporterExporter::sceneImporter
query the two reloaded entities from Manager::getSingleton() and assert they
resolve to the same material resource (compare material pointers/names). Use the
existing symbols MeshImporterExporter::sceneExporter,
MeshImporterExporter::sceneImporter, and Manager::getSingleton()/createEntity to
locate where to add the new assertion and fail the test if deduplication is not
observed.

src/MeshImporterExporter.cpp (1)

1577-1583: ⚠️ Potential issue | 🔴 Critical

Parsing first is only a partial fix here.

The live scene is still destroyed before loadScene(), skeleton reconstruction, and createMesh() have succeeded. Any exception on that path now returns false with the editor empty or partially rebuilt. Please stage the import and swap only after the whole scene is known-good.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 1577 - 1583, The current code
clears the live scene immediately (SelectionSet::getSingleton()->clearList() and
Manager::getSingleton()->destroySceneNode(...)) which can leave the editor empty
if loadScene(), skeleton reconstruction, or createMesh() later throw; instead,
stage the import into temporary structures and only swap into the live Manager
on success: build the new scene off-line (e.g., using a temporary Manager
instance or by constructing new scene nodes/meshes/skeletons into a local
container), run loadScene(), skeleton reconstruction, and createMesh() against
that staged data, and if all succeed replace the live Manager’s scene atomically
(clear and destroy existing nodes and move staged nodes into Manager) so
exceptions never leave the editor in a partially rebuilt state. Ensure
rollback/cleanup of staged resources on failure.

src/MeshImporterExporter.cpp (4)

1097-1106: ⚠️ Potential issue | 🟠 Major

Material deduplication key is unsafe across resource groups.

At Line 1097 and Line 1286, keying by mat->getName() alone can collapse distinct materials that share a name but live in different Ogre groups.

Proposed fix

-    std::map<std::string, unsigned int, std::less<>> matIndexMap;
+    using MaterialKey = std::pair<std::string, std::string>; // (group, name)
+    std::map<MaterialKey, unsigned int> matIndexMap;
@@
-            if (matIndexMap.find(mat->getName()) == matIndexMap.end())
+            MaterialKey key{mat->getGroup(), mat->getName()};
+            if (matIndexMap.find(key) == matIndexMap.end())
             {
-                matIndexMap[mat->getName()] = static_cast<unsigned int>(materials.size());
+                matIndexMap[key] = static_cast<unsigned int>(materials.size());
                 materials.push_back(mat);
             }
@@
-            auto matIt = matIndexMap.find(subEnt->getMaterial()->getName());
+            MaterialKey key{subEnt->getMaterial()->getGroup(), subEnt->getMaterial()->getName()};
+            auto matIt = matIndexMap.find(key);

Also applies to: 1286-1287

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 1097 - 1106, The material
deduplication currently keys matIndexMap by mat->getName(), which can
incorrectly merge materials with the same name from different resource groups;
update the key to include the material's resource group (e.g., combine
mat->getName() and mat->getGroup() or another unique group identifier) wherever
matIndexMap is populated/queried (reference symbols: matIndexMap, nodeEntities
loop using getSubEntities() and getMaterial(), and the materials vector) and
apply the same change to the other occurrence around the second block (the lines
referenced at 1286-1287) so materials are unique per resource group.

---

37-38: ⚠️ Potential issue | 🟡 Minor

Add a direct <functional> include for std::function.

std::function is used at Line 1207 and Line 1619, but this TU doesn’t include <functional> explicitly.

Minimal fix

 `#include` <QDebug>
 `#include` <set>
+#include <functional>

Also applies to: 1207-1207, 1619-1619

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 37 - 38, This translation unit
uses std::function in two places (around lines 1207 and 1619) but does not
include <functional>; add a direct `#include` <functional> to the top of
MeshImporterExporter.cpp alongside the other includes (e.g., after the existing
<set> include) so the uses of std::function compile reliably across toolchains.

---

1577-1583: ⚠️ Potential issue | 🟠 Major

Import is still non-transactional after parse success.

The current scene is cleared at Line 1577 before mesh/skeleton/material reconstruction completes; if later processing throws, users still lose the existing scene.

Also applies to: 1585-1926

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 1577 - 1583, The import clears the
live scene early (SelectionSet::getSingleton()->clearList(), emit
manager->sceneClearing(), manager->destroySceneNode(...)) so if reconstruction
throws later the original scene is lost; make the operation transactional by
building the imported meshes/skeletons/materials into a temporary container or
newly allocated scene nodes (not attached to the live Manager) and only call
SelectionSet::getSingleton()->clearList(), emit manager->sceneClearing(), and
manager->destroySceneNode(...) to remove the old nodes after the entire
reconstruction completes without exception; alternatively wrap reconstruction in
try/catch and on success swap the temporary nodes into Manager::getSingleton(),
on failure leave Manager untouched and clean up temporaries. Ensure the unique
symbols to change are the pre-clear sequence
(SelectionSet::getSingleton()->clearList(); emit manager->sceneClearing(); auto
sceneNodesCopy = manager->getSceneNodes(); for (auto* sn : sceneNodesCopy)
manager->destroySceneNode(sn);) and the code paths that allocate new scene nodes
so they use the temporary container and only get attached on successful
completion.

---

1195-1196: ⚠️ Potential issue | 🟠 Major

nodeName + "_" prefix scoping is ambiguous for similarly named entities.

Using _ as the namespace separator can cross-match unrelated entities (e.g., Hero_ also matches Hero_Alt_*), which can re-mix bones/animations during import.

Proposed fix (collision-resistant separator)

-        std::string bonePrefix = (nodeEntities.size() > 1 && hasSkeleton)
-            ? std::string(sn->getName()) + "_" : "";
+        static const std::string kEntitySep = "__NODE__";
+        std::string bonePrefix = (nodeEntities.size() > 1 && hasSkeleton)
+            ? std::string(sn->getName()) + kEntitySep : "";
@@
-                entityPrefix = nodeName.toStdString() + "_";
+                entityPrefix = nodeName.toStdString() + kEntitySep;

Also applies to: 1674-1677, 1715-1718, 1796-1801, 1875-1876

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 1195 - 1196, The current use of
"_" as the bone/namespace separator is ambiguous and can collide with similarly
named entities; replace the separator with a collision-resistant delimiter
(e.g., "::" or a rarely-used token) wherever bonePrefix is built (the code using
variable bonePrefix and sn->getName() when nodeEntities.size() > 1 &&
hasSkeleton) and in the other similar concatenations noted (the other
occurrences that construct names for bones/animations). Update all
matching/parsing code that later splits or compares those names to use the new
delimiter so prefix matching is exact (search for uses of bonePrefix,
concatenations with sn->getName(), and any name-splitting logic and change both
construction and parsing to the new separator).

src/mainwindow.cpp (3)

1387-1387: ⚠️ Potential issue | 🟡 Minor

Handle failed scene imports in openRecentFile().

Similar to on_actionOpen_Scene_triggered(), the return value of sceneImporter() is ignored here. A failed import should notify the user rather than silently failing.

🔧 Proposed fix

-        if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || 
-            filePath.endsWith(".scene.gltf", Qt::CaseInsensitive))
-            MeshImporterExporter::sceneImporter(filePath);
-        else
+        if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || 
+            filePath.endsWith(".scene.gltf", Qt::CaseInsensitive)) {
+            if (!MeshImporterExporter::sceneImporter(filePath)) {
+                QMessageBox::warning(this, tr("Open Scene"),
+                    tr("Failed to open scene file \"%1\".").arg(filePath));
+                return;
+            }
+        } else {
             mUriList.append(filePath);
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mainwindow.cpp` at line 1387, In openRecentFile() the call to
MeshImporterExporter::sceneImporter(filePath) currently ignores its return
value; update openRecentFile() to capture the boolean result from
MeshImporterExporter::sceneImporter(filePath), and if it returns false show a
user-visible error (e.g., QMessageBox::critical or a status bar message) and
abort further processing (return early), mirroring the behavior in
on_actionOpen_Scene_triggered(); ensure the error message gives context (e.g.,
"Failed to import scene: <filename>") so users are notified of the failure.

---

1386-1389: ⚠️ Potential issue | 🟡 Minor

Use case-insensitive comparison for scene file extensions.

The endsWith() calls don't use Qt::CaseInsensitive, so files like MyScene.SCENE.GLB or test.Scene.GltF won't be recognized as scene files and will be incorrectly processed as mesh imports.

🔧 Proposed fix

-        if (filePath.endsWith(".scene.glb") || filePath.endsWith(".scene.gltf"))
+        if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || 
+            filePath.endsWith(".scene.gltf", Qt::CaseInsensitive))
             MeshImporterExporter::sceneImporter(filePath);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mainwindow.cpp` around lines 1386 - 1389, The endsWith checks on filePath
are case-sensitive so files like "MyScene.SCENE.GLB" won't match; update the
logic where filePath is tested (the endsWith calls before calling
MeshImporterExporter::sceneImporter or mUriList.append) to perform
case-insensitive comparisons by passing Qt::CaseInsensitive (e.g.,
filePath.endsWith(".scene.glb", Qt::CaseInsensitive) and similarly for
".scene.gltf") so scene files are recognized regardless of extension case.

---

601-607: ⚠️ Potential issue | 🟠 Major

Handle failed scene imports before adding to recent files.

The return value of MeshImporterExporter::sceneImporter(fileName) is ignored. If the import fails (returns false), the file is still added to recent files at line 607, which could confuse users when re-opening a corrupted or invalid scene file.

🐛 Proposed fix

     auto txn = SentryReporter::startTransaction("ui.import", "scene.import");
+    bool imported = false;
     try {
-        MeshImporterExporter::sceneImporter(fileName);
+        imported = MeshImporterExporter::sceneImporter(fileName);
     } catch (...) {
         SentryReporter::finishTransaction(txn);
         throw;
     }
     SentryReporter::finishTransaction(txn);
+    if (!imported) {
+        QMessageBox::warning(this, tr("Open Scene"), tr("Failed to open scene."));
+        return;
+    }
     addToRecentFiles(fileName);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mainwindow.cpp` around lines 601 - 607,
MeshImporterExporter::sceneImporter(fileName) currently returns a success flag
that's ignored, so change the flow to capture its boolean result, call
SentryReporter::finishTransaction(txn) in all paths as now, and only call
addToRecentFiles(fileName) when the import succeeded (result == true); preserve
the existing catch(...) rethrow behavior and ensure
SentryReporter::finishTransaction(txn) is still invoked before rethrowing.

src/ViewCube/ViewCubeController.h (1)

68-68: Consider using QPointer<QQuickWidget> for consistency with m_activeWidget.

The m_activeWidget member uses QPointer<OgreWidget> for safe tracking when the widget is externally destroyed. While m_cubeWidget is owned by this controller (created in initWidget()), using QPointer would provide consistent null-safety semantics across widget members and guard against accidental double-delete scenarios.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ViewCube/ViewCubeController.h` at line 68, Replace the raw pointer member
m_cubeWidget with QPointer<QQuickWidget> to match m_activeWidget's null-safe
semantics: update the declaration in ViewCubeController to
QPointer<QQuickWidget> m_cubeWidget, ensure <QPointer> is included, and audit
usages (e.g., in initWidget() and any destruction/ownership code) to check for
.isNull()/.clear() or direct pointer access via operator->/operator*; this keeps
ownership behavior but guards against external deletion and accidental
double-delete.